revert: always rebuild frontend, override caching with _NEED_FRONTEND_BUILD=true#4427
Conversation
The mtime-based caching introduced in #4404/#4413 can incorrectly skip frontend builds -- e.g. after git pull when filesystem timestamps are not preserved, or after our Tailwind v4 discovery that the site-packages .gitignore must be hidden before vite build (which the cached path doesn't handle). Always rebuild the frontend on setup. The build takes ~15s and is safer than risking a stale dist/.
Caching disabled by always setting _NEED_FRONTEND_BUILD=true. The mtime-based logic is preserved in comments for future re-enabling. Reasons for disabling: - Git does not preserve file timestamps, so cached dist/ can appear newer than freshly checked-out source after a pull - Tailwind v4 requires hiding site-packages/.gitignore before vite build; the cache path bypasses this, producing broken CSS
danielhanchen
changed the title
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request removes the frontend build caching mechanism from the setup scripts. The caching, which aimed to skip frontend builds when no source files were newer than the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
danielhanchen
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request disables the frontend build caching by consistently setting the build flag to true. While this achieves the goal, the implementation deletes the caching logic entirely, which contradicts the pull request's title and description stating the code should be commented out for future re-enabling. I've provided suggestions to comment out the respective logic in both setup.sh and setup.ps1 to align with the stated intent and improve maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
studio/setup.ps1 (830)
The pull request description states that the caching logic should be preserved in comments for easy re-enabling. However, the elseif block containing this logic has been deleted. To align with the PR's intent, it's better to comment out this block.
}
# The mtime-based build cache is temporarily disabled. To re-enable, uncomment the following 'elseif' block.
# elseif (Test-Path (Join-Path $FrontendDir "dist")) {
# $DistDir = Join-Path $FrontendDir "dist"
# $DistTime = (Get-Item $DistDir).LastWriteTime
# $NewerFile = $null
# # Check src/ and public/ recursively (probe paths directly, not via -Include)
# foreach ($subDir in @("src", "public")) {
# $subPath = Join-Path $FrontendDir $subDir
# if (Test-Path $subPath) {
# $NewerFile = Get-ChildItem -Path $subPath -Recurse -File -ErrorAction SilentlyContinue |
# Where-Object { $_.LastWriteTime -gt $DistTime } | Select-Object -First 1
# if ($NewerFile) { break }
# }
# }
# # Also check all top-level files (package.json, bun.lock, vite.config.ts, index.html, etc.)
# if (-not $NewerFile) {
# $NewerFile = Get-ChildItem -Path $FrontendDir -File -ErrorAction SilentlyContinue |
# Where-Object { $_.LastWriteTime -gt $DistTime } |
# Select-Object -First 1
# }
# if (-not $NewerFile) {
# $NeedFrontendBuild = $false
# Write-Host "[OK] Frontend already built and up to date -- skipping build" -ForegroundColor Green
# } else {
# Write-Host "[INFO] Frontend source changed since last build -- rebuilding..." -ForegroundColor Yellow
# }
# }
studio/setup.sh (44)
The pull request description indicates that the caching logic should be commented out, but it has been deleted. To make it easier to re-enable this logic in the future, please preserve it within comments as intended.
_NEED_FRONTEND_BUILD=true
# The mtime-based build cache is temporarily disabled. To re-enable, uncomment the following block.
# if [ -d "$SCRIPT_DIR/frontend/dist" ]; then
# # Check all top-level files (package.json, bun.lock, vite.config.ts, index.html, etc.)
# _changed=$(find "$SCRIPT_DIR/frontend" -maxdepth 1 -type f \
# -newer "$SCRIPT_DIR/frontend/dist" -print -quit 2>/dev/null)
# # Check src/ and public/ recursively (|| true guards against set -e when dirs are missing)
# if [ -z "$_changed" ]; then
# _changed=$(find "$SCRIPT_DIR/frontend/src" "$SCRIPT_DIR/frontend/public" \
# -type f -newer "$SCRIPT_DIR/frontend/dist" -print -quit 2>/dev/null) || true
# fi
# if [ -z "$_changed" ]; then
# _NEED_FRONTEND_BUILD=false
# fi
# fi
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec3ee83cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _NEED_FRONTEND_BUILD=true | ||
| if [ -d "$SCRIPT_DIR/frontend/dist" ]; then | ||
| # Check all top-level files (package.json, bun.lock, vite.config.ts, index.html, etc.) | ||
| _changed=$(find "$SCRIPT_DIR/frontend" -maxdepth 1 -type f \ | ||
| -newer "$SCRIPT_DIR/frontend/dist" -print -quit 2>/dev/null) | ||
| # Check src/ and public/ recursively (|| true guards against set -e when dirs are missing) | ||
| if [ -z "$_changed" ]; then | ||
| _changed=$(find "$SCRIPT_DIR/frontend/src" "$SCRIPT_DIR/frontend/public" \ | ||
| -type f -newer "$SCRIPT_DIR/frontend/dist" -print -quit 2>/dev/null) || true | ||
| fi | ||
| if [ -z "$_changed" ]; then | ||
| _NEED_FRONTEND_BUILD=false | ||
| fi | ||
| fi | ||
| if [ "$_NEED_FRONTEND_BUILD" = false ]; then |
There was a problem hiding this comment.
This change removes the only dist short-circuit in setup.sh, so _NEED_FRONTEND_BUILD is now always true and Linux setup always runs Node/npm install + frontend rebuild. That regresses pip-installed Studio flows where a prebuilt frontend/dist is already shipped, because setup can now fail in environments that cannot fetch/install Node even though the bundled frontend is usable. Please keep a reliable pip/bundled-dist bypass before the Node/build phase.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8cce268b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _NEED_FRONTEND_BUILD=false | ||
| fi | ||
| fi | ||
| _NEED_FRONTEND_BUILD=true |
There was a problem hiding this comment.
This unconditional override disables the dist/ fast-path, so setup.sh now always executes the Node/npm + frontend rebuild path even when a prebuilt frontend is already bundled. unsloth studio setup resolves and runs studio/setup.sh from site-packages, and the wheel includes frontend/dist data, so pip-installed Linux environments that previously could skip frontend tooling now require Node/network access and can fail setup unnecessarily.
Useful? React with 👍 / 👎.
…_BUILD=true (unslothai#4427) * revert: remove frontend build caching from setup scripts The mtime-based caching introduced in unslothai#4404/unslothai#4413 can incorrectly skip frontend builds -- e.g. after git pull when filesystem timestamps are not preserved, or after our Tailwind v4 discovery that the site-packages .gitignore must be hidden before vite build (which the cached path doesn't handle). Always rebuild the frontend on setup. The build takes ~15s and is safer than risking a stale dist/. * revert: disable frontend build caching, keep code commented out Caching disabled by always setting _NEED_FRONTEND_BUILD=true. The mtime-based logic is preserved in comments for future re-enabling. Reasons for disabling: - Git does not preserve file timestamps, so cached dist/ can appear newer than freshly checked-out source after a pull - Tailwind v4 requires hiding site-packages/.gitignore before vite build; the cache path bypasses this, producing broken CSS * revert: always rebuild frontend, remove mtime caching * revert: always rebuild frontend, override caching with _NEED_FRONTEND_BUILD=true
1 participant
Summary
Adds a single
_NEED_FRONTEND_BUILD=trueline after the mtime-based caching block insetup.shandsetup.ps1, overriding whatever the cache check decided.The caching logic itself is left intact for future re-enabling -- just remove the override line.
Why
Mtime-based caching is unreliable in the current setup:
site-packages/.gitignorebeforevite build; the cache path can bypass this, producing broken CSS